Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NaN is a new builtin type #5744

Closed
wants to merge 11 commits into from
Closed

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Feb 22, 2023

Introduce new Nan builtin type that has Decimal as its super type.

@Akirathan Akirathan linked an issue Feb 22, 2023 that may be closed by this pull request
@Akirathan Akirathan self-assigned this Feb 22, 2023
@Akirathan Akirathan force-pushed the wip/akirathan/nan-is-new-type-5739 branch from 3ae8afe to 489ce95 Compare February 23, 2023 10:27
@Akirathan Akirathan marked this pull request as ready for review February 23, 2023 10:27
Comment on lines -944 to -947
Comparable.from (that:Number) =
case that.is_nan of
True -> Incomparable
False -> Default_Ordered_Comparator
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to be a huge performance bottleneck.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some speedup after the rewrite?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some speedup after the rewrite?

Yes. sieve.enso is 2x faster, and EqualsBenchmarks.equalsPrimitives is 10% faster. I have not tried other benchmarks, but I expect similar results, certainly no regressions.

Comment on lines -944 to -947
Comparable.from (that:Number) =
case that.is_nan of
True -> Incomparable
False -> Default_Ordered_Comparator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some speedup after the rewrite?

test/Tests/src/Semantic/Meta_Spec.enso Show resolved Hide resolved
Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for when NaN is somewhere in a Vector and you try to sort it?
That was a requirement by @jdunkerley IIRC

@Akirathan
Copy link
Member Author

Can you add a test for when NaN is somewhere in a Vector and you try to sort it? That was a requirement by @jdunkerley IIRC

This will be tackled in #5742, in a follow-up PR.

@Akirathan Akirathan force-pushed the wip/akirathan/nan-is-new-type-5739 branch from 3275867 to d7e715d Compare February 23, 2023 16:33
@Akirathan Akirathan mentioned this pull request Feb 24, 2023
2 tasks
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good.

I'm hesitant of expanding our numeric type hierarchy like that, it feels like it will drive up the complexity of the type system and anything that relies on that hierarchy.

But it seems to work for what we need right now, so not arguing against it.

@JaroslavTulach
Copy link
Member

I'm hesitant of expanding our numeric type hierarchy like that,

+1 - Do Not Expose Deep Hierarchies, Practical API Design page 83

it feels like it will drive up the complexity of the type system

Do you mean implementation? No, that's OK.

I should probably note that the core flaw is our "attempt to not support partial order on any type". If our comparator method could return:

compare :: T -> T -> Ordering | Nothing

we wouldn't have this problem. I am OK with the introduction of Nan type, but I would much rather see us supporting "partial ordering" by default. E.g. return the design back where I left it:

Then we wouldn't need to create an artificial type like Nan.

@Akirathan
Copy link
Member Author

I should probably note that the core flaw is our "attempt to not support partial order on any type". If our comparator method could return:
compare :: T -> T -> Ordering | Nothing
we wouldn't have this problem. I am OK with the introduction of Nan type, but I would much rather see us supporting "partial ordering" by default. E.g. return the design back where I left it:
#5008
Then we wouldn't need to create an artificial type like Nan.

Let's stick with the current design for now and see where it takes us. I assume that we will know for sure during the work on #5626. Potential rollback to compare: T -> T -> (Ordering | Nothing) should not be that difficult. After all, even stdlib has not fully migrated yet.

@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Feb 24, 2023
@Akirathan Akirathan removed the CI: Ready to merge This PR is eligible for automatic merge label Feb 25, 2023
@Akirathan Akirathan closed this Mar 1, 2023
@Akirathan Akirathan deleted the wip/akirathan/nan-is-new-type-5739 branch March 11, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NaN is a separate type
4 participants